Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
- Coverage 80.39% 80.19% -0.21%
==========================================
Files 47 47
Lines 1974 2065 +91
Branches 192 205 +13
==========================================
+ Hits 1587 1656 +69
- Misses 334 353 +19
- Partials 53 56 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
benoit74
left a comment
benoit74
left a comment
There was a problem hiding this comment.
Very good, thank you. I've identified few small flaws and I will open few more issues for stuff not yet identified as necessary for a smooth user experience.
Fix book status when book is pending deletion (both in book details and title details views), we should not have "Unknown" shown here:
Also when I mark a book for immediate "force deletion", the "recover" button is still shown (this might be acceptable) but the API call fails (this is expected) and the message says "Book ... is not eligible for deletion" (this is incorrect)
Display location_kind next to the green checkmark (in addition to the tooltip, both in book details and title details views):
When book is deleted, status is back to "Published" (in the UI) while it should be "Deleted" (both in book details and title details view)
| f"Book {book_id} does not meet criteria to be deleted." | ||
| ) | ||
|
|
||
| deletion_date = now if force_delete else now + Context.old_book_deletion_delay |
There was a problem hiding this comment.
Rename old_book_deletion_delay to book_deletion_delay (including env var)
| deletion_date = now if force_delete else now + Context.old_book_deletion_delay | ||
|
|
||
| if book.location_kind == "quarantine": | ||
| if book.has_error: |
There was a problem hiding this comment.
Let's simplify this logic and always delete books at deletion_date, looking again at this, it is going to be too confusing for users to have two distinct behaviors. Sorry for the late change.
| for loc in current_locations: | ||
| if loc.warehouse_id == Context.quarantine_warehouse_id: | ||
| try: | ||
| loc.path.relative_to(Context.quarantine_base_path) |
There was a problem hiding this comment.
Why not using is_relative_to which would avoid the try/except block?
| ) | ||
| logger.exception(f"Failed to delete files for book {book.id}") | ||
| book.has_error = True | ||
| book.needs_file_operation = False |
There was a problem hiding this comment.
I'm a bit embarrassed by this because it means we will never see that a book which is supposed to be deleted is still present on the filesystem (because tbh, nobody will go have a look after events and logs if not alarmed by some flag).
Could you explain again what is the downside of setting has_error to True? I don't mind about setting needs_file_operation to False, this is ok.
There was a problem hiding this comment.
Okay. I forgot that I no longer use has_error in the logic to get_next_book_to_delete. So, it can be safely added here.
| if not book: | ||
| break | ||
|
|
||
| logger.info("Moving ZIM files") |
There was a problem hiding this comment.
Why making this log at every book we move?
There was a problem hiding this comment.
Mistake. Logs kept appearing even when no book was being moved. Moved it now to be Moving ZIM files for book {book_id} when something is actually being moved.
| </v-alert> | ||
|
|
||
| <div v-if="book" class="mt-4 pa-3 bg-grey-lighten-4 rounded"> | ||
| <div class="text-caption text-grey mb-1">Book ID:</div> |
There was a problem hiding this comment.
Please display as well (no need for the copy button for these fields) the book name, flavour, date and created_at.
Maybe something more "condensed" like on "recover" popup would look better, especially on small screens. Not sure about that tbh.
| <v-dialog v-model="isOpen" max-width="600px" persistent> | ||
| <v-card> | ||
| <v-card-title class="text-h5 pa-4 bg-primary"> | ||
| <span class="text-white">Move Book</span> |
There was a problem hiding this comment.
Please display again the book name, flavour, date and created_at ; can probably help to avoid mistakes.
| This will recover the book and cancel the deletion process. | ||
| </v-alert> | ||
|
|
||
| <div v-if="book" class="mb-4"> |
There was a problem hiding this comment.
Please remove the book ID (no-one will check that) but display as well the book date and created_at ; can probably help to avoid mistakes.
| </v-alert> | ||
|
|
||
| <v-select | ||
| v-model="destination" |
There was a problem hiding this comment.
When there is only one available destination (which is usually the case), then this destination should be automatically selected.
2 participants
Rationale
This PR adds functionality to move, recover and delete books from the API/UI.
Changes
get_book_or_nonefunction to allow for passing params for filtering instead of havingif...elsechecks for attributesdelete_fileslogic set the bookhas_errorif deletion failed. This doesn't seem correct as the book does not necessarily have an error. Instead, in addition to logging the exception message, set theneeds_file_operationto Falsehas_errorflag from filters to get_next_book_to_delete as we want to now delete books that have errors tooquarantine,stagingand/orprodbooks in library.xml generation (filter added in DB queryget_latest_books_for_collection)This closes #143